Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added simple advanced search page #134

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

wadeking98
Copy link
Contributor

Added ability to search by credential type
Resolves: #70
Signed-off-by: wadeking98 wkingnumber2@gmail.com

@amanji
Copy link
Collaborator

amanji commented Jan 25, 2022

@swcurran Is this the intended implementation? I recall the discussion being something around linking to search results by type from the Data Types (About) section. I'm of the view that if we want to add a separate advanced search feature, perhaps we should iterate on the current search feature. Thoughts? What about you @wadeking98?

@WadeBarnes
Copy link
Member

@amanji, Based on this comment I would say this is not the intended implementation; #70 (comment)

@swcurran Is this the intended implementation? I recall the discussion being something around linking to search results by type from the Data Types (About) section. I'm of the view that if we want to add a separate advanced search feature, perhaps we should iterate on the current search feature. Thoughts? What about you @wadeking98?

@swcurran
Copy link
Contributor

See my notes on the issue #70 -- we don't want to have this implemented in this way. We don't really want an "Advanced Search" at all, unless it provides features that are more like one would expect in an advanced search. This is definitely not that.

@amanji
Copy link
Collaborator

amanji commented Jan 25, 2022

Ah, I should have read through the issue discussion. I agree with @swcurran on his comments. The link-based approach is much cleaner for now until we iterate on the existing search bar.

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@wadeking98
Copy link
Contributor Author

@swcurran @amanji @WadeBarnes I've refactored the changes now and made it in line with Stephen's suggestion of having links on the page

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please provide a screenshot of what it looks like? I'm not set up to run the app locally.

Thanks!

@swcurran swcurran merged commit 68af9f6 into bcgov:main Jan 31, 2022
@wadeking98
Copy link
Contributor Author

@swcurran here is what the page looks like:
image

@swcurran
Copy link
Contributor

How about one more tweak. Title that "Credential Search by Type" vs. "Credentials"? I think that makes it a little clearer...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression -- being able to search for all credentials of a given type
4 participants